src/new/types.ts (1)

330-344: Model shielded outputs as a separate variant.

shielded?: ShieldedOutputMode documents the invariant, but it still allows a legacy/base58 address plus shielded: FULLY_SHIELDED to type-check. Since this is a public API, a discriminated union would catch that misuse earlier.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/new/types.ts` around lines 330 - 344, ProposedOutput currently allows any
address with an optional shielded flag so code can wrongly pair a legacy/base58
address with shielded modes; replace ProposedOutput with a discriminated union
(e.g. LegacyOutput and ShieldedOutput) so the discriminator is the shielded
field: LegacyOutput has shielded?: undefined and accepts legacy Address type,
while ShieldedOutput has shielded: ShieldedOutputMode (or a specific literal
like "AMOUNT_SHIELDED"|"FULLY_SHIELDED") and its address type is the
ShieldedAddress type (or otherwise constrained); update usages expecting
ProposedOutput to accept the union and ensure ShieldedOutput includes the same
value/timelock/token properties.

src/schemas.ts (1)

87-111: Consider using a discriminated union for stricter validation.

Making all fields optional accommodates both shapes but loses validation strictness. A shielded entry could pass without required shielded fields (e.g., commitment), or a transparent output could pass without value.

Consider using z.discriminatedUnion on the type field to enforce shape-specific requirements:

♻️ Suggested approach using discriminated union

const TransparentOutputSchema = z.object({
  value: bigIntCoercibleSchema,
  token_data: z.number(),
  script: z.string(),
  decoded: IHistoryOutputDecodedSchema,
  token: z.string(),
  spent_by: z.string().nullable().optional(),
  selected_as_input: z.boolean().optional(),
}).passthrough();

const ShieldedOutputSchema = z.object({
  type: z.literal('shielded'),
  commitment: z.string(),
  range_proof: z.string(),
  ephemeral_pubkey: z.string(),
  asset_commitment: z.string(),
  surjection_proof: z.string(),
}).passthrough();

export const IHistoryOutputSchema = z.union([
  TransparentOutputSchema,
  ShieldedOutputSchema,
]) as ZodSchema<IHistoryOutput>;

If the fullnode can return partially populated shielded entries or mixed shapes that don't fit a discriminated union, the current permissive approach is acceptable.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/schemas.ts` around lines 87 - 111, The schema IHistoryOutputSchema
currently marks all fields optional which weakens validation and allows
malformed transparent or shielded entries; update it to a discriminated union
based on the type field by creating two schemas (e.g., TransparentOutputSchema
and ShieldedOutputSchema) where TransparentOutputSchema requires fields like
value (bigIntCoercibleSchema) and related transparent properties and
ShieldedOutputSchema requires type: z.literal('shielded') and required shielded
fields such as commitment, range_proof, ephemeral_pubkey, asset_commitment,
surjection_proof, then export IHistoryOutputSchema as
z.union([TransparentOutputSchema, ShieldedOutputSchema]) (retain .passthrough()
if needed) so each shape is strictly validated.

src/utils/transaction.ts (1)

83-84: Prefer undefined over null for optional state here.

These new null sentinel types go against the repo-wide TS migration rule and make the helper/cache state less consistent with the rest of the module.

Based on learnings, avoid using null to indicate optional/unprovided parameters in TypeScript; use undefined and the ? optional syntax instead.

Also applies to: 193-194

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/transaction.ts` around lines 83 - 84, The helper is using null as a
sentinel for optional state; update the parameter and checks to use undefined
instead: change the type annotations from `{ type?: string | null }` to `{
type?: string }` in isShieldedOutputEntry and the other occurrence around the
code flagged (lines ~193-194), and update any strict equality checks or guards
that compare to null to instead check for undefined or use truthiness
(`output.type !== undefined` or `!!output.type`) so the module follows the
repo-wide TS convention of using undefined for optional/unprovided values.

__tests__/integration/configuration/docker-compose.yml (1)

11-11: Pin these shielded test images by digest to ensure reproducible integration environments.

Both new tags are mutable prerelease-style labels. If either image is republished, the integration environment can change underneath the same commit and make shielded test failures harder to reproduce locally or in CI. The images can be pinned by their digests instead:

• hathornetwork/hathor-core:experimental-shielded-outputs-alpha-v1@sha256:c1d372698f0b549dbb2e2e6955ae27ec1f9cf728948c6d758651636b4c3ff5f5
• hathornetwork/tx-mining-service:shielded-outputs-v1@sha256:9e78ce25741fa07b092c5ab55ad18d6f76f40a28d1b28e6580bb7fd133d511b4

This applies to lines 11 and 45.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@__tests__/integration/configuration/docker-compose.yml` at line 11, Replace
the mutable image tags with their content-addressable digests to make
integration tests reproducible: update the value currently using
"hathornetwork/hathor-core:experimental-shielded-outputs-alpha-v1" (referenced
via ${HATHOR_LIB_INTEGRATION_TESTS_FULLNODE_IMAGE:-...}) to
"hathornetwork/hathor-core:experimental-shielded-outputs-alpha-v1@sha256:c1d372698f0b549dbb2e2e6955ae27ec1f9cf728948c6d758651636b4c3ff5f5"
and likewise pin "hathornetwork/tx-mining-service:shielded-outputs-v1" to
"hathornetwork/tx-mining-service:shielded-outputs-v1@sha256:9e78ce25741fa07b092c5ab55ad18d6f76f40a28d1b28e6580bb7fd133d511b4"
so the Docker Compose entries for those images are fixed by digest.

src/storage/memory_store.ts (1)

469-476: Consider defining a proper type for shielded output detection.

The cast (output as any).type !== 'shielded' works but bypasses type safety. Consider extending the output type in IHistoryTx or using a type guard function for cleaner detection.

♻️ Example type guard

// In a types or utils file
function isShieldedHistoryOutput(output: unknown): boolean {
  return (output as { type?: string }).type === 'shielded';
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/storage/memory_store.ts` around lines 469 - 476, The code uses an unsafe
cast "(output as any).type !== 'shielded'" to detect shielded outputs; instead
add a proper type or type guard and use it: extend the output type in IHistoryTx
(or create a dedicated IHistoryOutput with optional "type?: string") and replace
the any-cast check with a type guard function (e.g.,
isShieldedHistoryOutput(output): boolean) or a typed property check so the
condition in the loop uses the guard (or the typed property) to determine
shielded outputs rather than casting to any; update references in the loop
around the variables "output", "IHistoryTx", and the file "memory_store.ts" to
call the new guard/typed property.

src/models/shielded_output.ts (1)

44-64: Consider using an options object for constructor parameters.

The constructor has 9 positional parameters with value positioned between assetCommitment and surjectionProof. This ordering is error-prone and the current signature makes it easy to accidentally swap assetCommitment and value. Consider using a named options object pattern.

♻️ Example refactor using options object

-  constructor(
-    mode: ShieldedOutputMode,
-    commitment: Buffer,
-    rangeProof: Buffer,
-    tokenData: number,
-    script: Buffer,
-    ephemeralPubkey: Buffer,
-    assetCommitment?: Buffer,
-    value: OutputValueType = 0n,
-    surjectionProof?: Buffer,
-  ) {
+  constructor(opts: {
+    mode: ShieldedOutputMode;
+    commitment: Buffer;
+    rangeProof: Buffer;
+    tokenData: number;
+    script: Buffer;
+    ephemeralPubkey: Buffer;
+    assetCommitment?: Buffer;
+    surjectionProof?: Buffer;
+    value?: OutputValueType;
+  }) {
+    this.mode = opts.mode;
+    this.commitment = opts.commitment;
+    // ... etc
+    this.value = opts.value ?? 0n;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/models/shielded_output.ts` around lines 44 - 64, The constructor on class
ShieldedOutput accepts many positional args (mode, commitment, rangeProof,
tokenData, script, ephemeralPubkey, assetCommitment, value, surjectionProof)
which is error-prone (especially assetCommitment vs value); refactor the
constructor to accept a single options object (e.g., { mode, commitment,
rangeProof, tokenData, script, ephemeralPubkey, assetCommitment?, value?,
surjectionProof? }) and update the constructor body to pull values from that
object, update any call sites to pass a named options object, and keep the same
property names on the instance (this.mode, this.commitment,
this.assetCommitment, this.value, this.surjectionProof) to preserve behavior.

src/storage/storage.ts (1)

83-91: Consider clearing pinCode after use for better security hygiene.

Storing pinCode in memory on the Storage instance means it persists for the lifetime of the wallet session. While this may be intentional for convenience during shielded output decryption, consider whether the pin should be cleared after processing completes or after a timeout, to reduce the window of exposure if the process memory is compromised.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/storage/storage.ts` around lines 83 - 91, The Storage class currently
keeps the pinCode property in memory for the wallet lifetime; update the code so
any method that consumes pinCode (e.g., the shielded output decryption routines
that use shieldedCryptoProvider / the promise used to chain processing of
unlocked UTXOs) clears this.pinCode when finished — use a try/finally around the
decryption/processing logic to set this.pinCode = undefined (or null) in the
finally block, and optionally add an async timeout-based clear (clear after X
ms) where pinCode is set to reduce exposure.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/headers/shielded_outputs.ts (1)

53-109: ⚠️ Potential issue | 🟠 Major

Reject malformed shielded headers instead of slicing through them.

Every field here is read with subarray() and no bounds checks, and unsupported mode values still fall through to the ephemeral-pubkey read. A truncated or unknown payload can desync buf and deserialize as a bogus header instead of failing fast. Apply the same guard to every fixed-size and length-prefixed read.

🛠️ Suggested direction

       let mode: number;
       [mode, buf] = unpackToInt(1, false, buf);
+      if (
+        mode !== ShieldedOutputMode.AMOUNT_SHIELDED &&
+        mode !== ShieldedOutputMode.FULLY_SHIELDED
+      ) {
+        throw new Error(`Unsupported shielded output mode: ${mode}`);
+      }
 
       // Commitment (33 bytes)
+      if (buf.length < 33) {
+        throw new Error('Truncated shielded output commitment');
+      }
       const commitment = Buffer.from(buf.subarray(0, 33));
       buf = buf.subarray(33);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/headers/shielded_outputs.ts` around lines 53 - 109, The deserialize
function is vulnerable to out-of-bounds reads and silent desyncs; before every
fixed-size read (33-byte commitments, ephemeralPubkey, assetCommitment) and
before consuming variable-length fields (after unpackToInt for rpLen, scriptLen,
spLen) assert buf.length >= requiredSize and throw an Error if not, and
explicitly validate the mode read from unpackToInt against ShieldedOutputMode
(handle AMOUNT_SHIELDED and FULLY_SHIELDED, otherwise throw) so unsupported
modes do not fall through; update logic in deserialize, referencing
getVertexHeaderIdFromBuffer, unpackToInt, ShieldedOutputMode, and the local
variables commitment, rangeProof, script, assetCommitment, surjectionProof,
ephemeralPubkey to perform these length checks and fail fast on malformed input.

src/new/wallet.ts (1)

1477-1502: ⚠️ Potential issue | 🔴 Critical

Preserve on-chain output indexes.

Replacing newTx.outputs with a compacted transparent-only array renumbers every transparent output after the first shielded entry. The rest of the wallet still uses array position as the canonical output index, so mixed transactions will save/spend the wrong UTXOs.

🛠️ Suggested fix

     if (!newTx.shielded_outputs) {
       const shieldedEntries: IHistoryShieldedOutput[] = [];
-      const transparentOutputs: IHistoryOutput[] = [];
       for (const output of newTx.outputs) {
         if (transactionUtils.isShieldedOutputEntry(output)) {
           shieldedEntries.push({
             mode: output.asset_commitment ? 2 : 1, // FullShielded=2, AmountShielded=1
             commitment: output.commitment!, // already hex
@@
             surjection_proof: output.surjection_proof
               ? Buffer.from(output.surjection_proof, 'base64').toString('hex')
               : undefined,
           });
-        } else {
-          transparentOutputs.push(output);
         }
       }
       if (shieldedEntries.length > 0) {
         newTx.shielded_outputs = shieldedEntries;
-        newTx.outputs = transparentOutputs;
       }
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/new/wallet.ts` around lines 1477 - 1502, The current code compacts
newTx.outputs into transparentOutputs, which shifts array indexes and breaks
UTXO indexing; instead, keep newTx.outputs intact and only extract shielded
entries into newTx.shielded_outputs without reassigning newTx.outputs. Locate
the block using newTx, shielded_outputs, outputs and
transactionUtils.isShieldedOutputEntry and change the logic so you push shielded
entries into shieldedEntries but do not replace newTx.outputs (or if you must
remove shielded entries, replace them with a stable placeholder object that
preserves the original array length/indexes) and then set newTx.shielded_outputs
= shieldedEntries when non-empty.

src/utils/storage.ts (1)

328-347: ⚠️ Potential issue | 🟠 Major

Ignore shielded cursors when shielded derivation is disabled.

These calculations still fold shieldedLastLoadedAddressIndex into Math.min(...) / target selection even when the wallet has no shielded keys. With the default -1, both policies restart from index 0 and re-fetch the whole prefix instead of only the missing suffix.

🛠️ Suggested direction

-  const lastLoaded = Math.min(lastLoadedAddressIndex, shieldedLastLoadedAddressIndex);
+  const hasShieldedKeys = !!(await storage.getAccessData())?.spendXpubkey;
+  const effectiveShieldedLastLoaded = hasShieldedKeys
+    ? shieldedLastLoadedAddressIndex
+    : lastLoadedAddressIndex;
+  const lastLoaded = Math.min(lastLoadedAddressIndex, effectiveShieldedLastLoaded);

Use the same guard for shieldedTarget / minLastLoaded in checkGapLimit().

Also applies to: 378-399

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/storage.ts` around lines 328 - 347, The code currently uses
Math.min(lastLoadedAddressIndex, shieldedLastLoadedAddressIndex) even when
shielded derivation is disabled (shieldedLastLoadedAddressIndex is -1), causing
unnecessary re-fetches; update the logic where lastLoaded/minLastLoaded and
shieldedTarget are computed to ignore shieldedLastLoadedAddressIndex when it's
negative (treat <0 as "no shielded keys") — e.g., in the block using
lastLoadedAddressIndex and shieldedLastLoadedAddressIndex replace the Math.min
call with a guarded computation that uses shieldedLastLoadedAddressIndex only if
shieldedLastLoadedAddressIndex >= 0, and apply the same guard in checkGapLimit()
where shieldedTarget/minLastLoaded are calculated so shielded cursors are
skipped when shielded derivation is disabled.

src/types.ts (2)

657-660: ⚠️ Potential issue | 🟠 Major

Allow clearing cached PIN via the interface.

Line 658 declares pinCode?: string, but Line 659 requires a string argument. This prevents explicit in-memory secret clearing through the API.

Minimal fix

-  setPinCode(pinCode: string): void;
+  setPinCode(pinCode?: string): void;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/types.ts` around lines 657 - 660, The interface currently defines
pinCode?: string but setPinCode(pinCode: string): void requires a string,
preventing callers from clearing the cached PIN; change the setPinCode signature
to accept an optional/nullable value (e.g. setPinCode(pinCode?: string | null):
void) and update any implementations of setPinCode and callers to handle
undefined/null as “clear the in-memory PIN” while preserving existing behavior
when a string is passed; update types in src/types.ts for the pinCode and
setPinCode symbols accordingly.

---

720-724: ⚠️ Potential issue | 🟠 Major

getSpendXPrivKey return type should model old-wallet absence.

spendMainKey is optional in IWalletAccessData (Line 446), so Line 722 should not guarantee Promise<string>.

Minimal fix

-  getSpendXPrivKey(pinCode: string): Promise<string>;
+  getSpendXPrivKey(pinCode: string): Promise<string | undefined>;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/types.ts` around lines 720 - 724, The TypeScript interface declares
getSpendXPrivKey(pinCode: string): Promise<string> but IWalletAccessData marks
spendMainKey as optional, so update the getSpendXPrivKey signature to return
Promise<string | undefined> to model absence of the old-wallet key; locate and
change the method declaration for getSpendXPrivKey in the interface in
src/types.ts (and adjust any dependent types/usages to handle undefined) to keep
signatures consistent with IWalletAccessData.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/types.ts (2)

737-741: ⚠️ Potential issue | 🟠 Major

Return type mismatch for getSpendXPrivKey.

The spendMainKey field is optional in IWalletAccessData, meaning older wallets won't have it. However, getSpendXPrivKey() returns Promise<string> rather than Promise<string | undefined>, which doesn't encode this possibility. The public key accessors (getScanXPubKey, getSpendXPubKey) correctly return Promise<string | undefined>.

-  getSpendXPrivKey(pinCode: string): Promise<string>;
+  getSpendXPrivKey(pinCode: string): Promise<string | undefined>;

Also apply the same fix to getScanXPrivKey for consistency.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/types.ts` around lines 737 - 741, The return types for the shielded
private-key accessors are incorrect: change getScanXPrivKey(pinCode: string) and
getSpendXPrivKey(pinCode: string) to return Promise<string | undefined>
(matching getScanXPubKey/getSpendXPubKey and the optional spendMainKey in
IWalletAccessData) so callers can handle missing keys; update the method
signatures in src/types.ts accordingly (keep the parameter names the same).

---

674-676: ⚠️ Potential issue | 🟠 Major

setPinCode should accept optional parameter to allow clearing.

The pinCode property is optional (pinCode?: string), but setPinCode(pinCode: string) requires a value, providing no interface-level way to clear the cached PIN. This unnecessarily extends the secret's lifetime in memory.

-  setPinCode(pinCode: string): void;
+  setPinCode(pinCode?: string): void;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/types.ts` around lines 674 - 676, The setter signature for pinCode is too
strict: the interface has pinCode?: string but setPinCode(pinCode: string)
requires a non-optional value, preventing callers from clearing the PIN; change
setPinCode to accept an optional value (e.g., pinCode?: string or pinCode:
string | undefined) on the interface/type that declares pinCode and setPinCode
(referencing pinCode and setPinCode in src/types.ts), and then update any
implementing classes/functions and their callers to pass undefined or omit the
argument when clearing the cached PIN.

src/utils/transaction.ts (1)

668-696: ⚠️ Potential issue | 🟠 Major

Missing validation for FULLY_SHIELDED-specific fields.

The validation only checks common fields (commitment, rangeProof, script, ephemeralPubkey). A FULLY_SHIELDED entry missing assetCommitment or surjectionProof will pass validation here but fail during serialization.

Add mode-specific validation:

         if (!so.commitment || !so.rangeProof || !so.script || !so.ephemeralPubkey) {
           throw new Error('Shielded output missing required crypto fields (commitment, rangeProof, script, ephemeralPubkey)');
         }
+        if (
+          so.mode === ShieldedOutputMode.FULLY_SHIELDED &&
+          (!so.assetCommitment || !so.surjectionProof)
+        ) {
+          throw new Error(
+            'Fully shielded output missing required crypto fields (assetCommitment, surjectionProof)'
+          );
+        }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/transaction.ts` around lines 668 - 696, The current validation in
the txData.shieldedOutputs mapping only enforces common crypto fields and misses
mode-specific requirements; update the validation inside the
txData.shieldedOutputs.map callback (where ShieldedOutput is constructed) to
additionally check when so.mode === 'FULLY_SHIELDED' that so.assetCommitment and
so.surjectionProof are present (non-null/defined) and throw a clear Error if
they are missing so construction/serialization won't fail later.

src/shielded/provider.ts (2)

29-35: ⚠️ Potential issue | 🟠 Major

Missing validation for negative bigint values.

The range check only rejects values above MAX_SAFE_INTEGER but allows negative values through. Since these represent output amounts, negative values should also be rejected to prevent unexpected behavior.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/shielded/provider.ts` around lines 29 - 35, The
createAmountShieldedOutput function currently only checks for values above
Number.MAX_SAFE_INTEGER but allows negative BigInt inputs; add a validation to
reject value < 0n and throw a clear Error (e.g., "Shielded output value must be
non-negative") before calling ct.createAmountShieldedOutput. Update the guard
around value (the parameter named value used in createAmountShieldedOutput and
passed to ct.createAmountShieldedOutput) to enforce 0n <= value <=
BigInt(Number.MAX_SAFE_INTEGER) and keep the existing error behavior for
out-of-range values.

---

87-92: ⚠️ Potential issue | 🟠 Major

toSafeNumber helper also misses negative check.

The helper function defined in computeBalancingBlindingFactor has the same issue - it should also reject negative values for inputs/outputs.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/shielded/provider.ts` around lines 87 - 92, The helper toSafeNumber
currently only checks for values greater than Number.MAX_SAFE_INTEGER but misses
negative values; update the toSafeNumber used in the shielded code (and the
duplicate helper inside computeBalancingBlindingFactor) to also validate that v
is >= 0 and throw a clear Error (e.g., 'Shielded output value out of safe
range') when v is negative or exceeds MAX_SAFE_INTEGER so both conversion sites
reject negative bigint inputs.

src/utils/wallet.ts (2)

704-719: ⚠️ Potential issue | 🟠 Major

Same multisig concern applies to generateAccessDataFromSeed.

This function unconditionally derives shielded spend keys even when walletType === WalletType.MULTISIG. Apply the same guard as suggested for generateAccessDataFromXpriv.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/wallet.ts` around lines 704 - 719, In generateAccessDataFromSeed,
avoid unconditionally deriving shielded spend keys when walletType ===
WalletType.MULTISIG: wrap the spendAcctXpriv/spendXpriv derivation and the use
of spendXpubkey and spendMainKey (and encryptData(spendXpriv.xprivkey, pin))
behind a guard that skips derivation for multisig wallets and instead returns
null/undefined (or an appropriate empty value) for spendXpubkey and
spendMainKey; reference the existing symbols spendAcctXpriv, spendXpriv,
spendXpubkey, spendMainKey, walletType and WalletType.MULTISIG to locate and
change the logic.

---

623-631: ⚠️ Potential issue | 🟠 Major

Shielded keys are derived for all wallet types including multisig.

This code derives single-sig shielded keys (m/44'/280'/1'/0) even for WalletType.MULTISIG wallets. This could break the multisig signing model if these keys are ever used, as one signer would control them alone.

Consider guarding against multisig:

-    if (argXpriv.depth === 0) {
+    if (argXpriv.depth === 0 && walletType === WalletType.P2PKH) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/wallet.ts` around lines 623 - 631, The current block
unconditionally derives shielded spend keys from argXpriv (using
SHIELDED_SPEND_ACCT_PATH and storing into accessData.spendXpubkey /
accessData.spendMainKey), which will produce single-sig shielded keys even for
multisig wallets; update the logic to skip deriving/encrypting these keys when
the wallet type is WalletType.MULTISIG (or any non-single-sig type) by checking
the wallet type before calling deriveNonCompliantChild and encryptData, so
multisig flows do not get private shielded keys created for a single signer.

src/models/transaction.ts (1)

369-372: ⚠️ Potential issue | 🟠 Major

shieldedOutputs is not populated during deserialization.

The getOutputsSum() method now includes this.shieldedOutputs, but createFromBytes() and getHeadersFromBytes() never rebuild this array from the serialized ShieldedOutputsHeader. Transactions parsed from bytes will have an empty shieldedOutputs array, causing incorrect weight calculations and validation.

Either populate shieldedOutputs from the header during deserialization, or derive the sum directly from the parsed header.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/models/transaction.ts` around lines 369 - 372, createFromBytes() and
getHeadersFromBytes() currently ignore the parsed ShieldedOutputsHeader so
this.shieldedOutputs stays empty; update those deserializers to rebuild/populate
the shieldedOutputs array from the parsed ShieldedOutputsHeader (or at minimum
compute and set the shielded outputs sum) so getOutputsSum() sees the real
shielded values. Specifically, in getHeadersFromBytes()/createFromBytes(),
locate where ShieldedOutputsHeader is parsed and either (a) iterate the header
entries to create shielded output objects (with .value populated) and assign
them to this.shieldedOutputs, or (b) derive and store the shielded sum from the
header into a field used by getOutputsSum(); ensure the produced shielded output
objects match the shape expected by getOutputsSum().

src/new/wallet.ts (1)

864-872: ⚠️ Potential issue | 🟠 Major

Return a shielded-chain path for shielded current addresses.

When opts?.legacy === false, this method can return a shielded address but still derives addressPath through getAddressPathForIndex(), which only builds transparent P2PKH/P2SH paths. Any caller that displays or re-derives from the returned path gets the wrong chain. getNextAddress() inherits the same bug.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/new/wallet.ts` around lines 864 - 872, The current getCurrentAddress (and
likewise getNextAddress) always computes addressPath via
getAddressPathForIndex(), which only builds transparent P2PKH/P2SH paths and
thus returns incorrect paths for shielded addresses when opts?.legacy === false;
update both methods to detect shielded addresses (e.g., via opts?.legacy flag or
the address format returned from storage.getCurrentAddress), and when a shielded
address is returned call the appropriate shielded-path builder (e.g.,
getShieldedAddressPathForIndex or equivalent) or otherwise compute a shielded
path instead of getAddressPathForIndex; also handle null/undefined index from
getAddressIndex() gracefully and keep transparent flows unchanged.

src/headers/shielded_outputs.ts (1)

67-109: ⚠️ Potential issue | 🟠 Major

Reject truncated and unsupported shielded entries during parsing.

This loop still slices fixed-size and length-prefixed fields without checking the remaining bytes, and it still accepts unknown mode values by falling through to the ephemeral-pubkey read. A short or invalid header can therefore build undersized ShieldedOutputs or misalign the buffer for the next entry instead of failing fast.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/headers/shielded_outputs.ts` around lines 67 - 109, The parsing loop for
shielded outputs must reject truncated or unknown entries: before each
fixed-size slice (commitment, assetCommitment, ephemeralPubkey) and before each
length-prefixed read (rangeProof, script, surjectionProof) check that buf.length
is >= the required bytes and that unpackToInt returned a sane length
(non-negative and <= buf.length); if not, throw a parse error. Also validate the
mode value read via unpackToInt against known ShieldedOutputMode values
(ShieldedOutputMode.AMOUNT_SHIELDED and ShieldedOutputMode.FULLY_SHIELDED) and
throw on unknown modes instead of falling through. Apply these checks around the
unpackToInt calls and the subsequent Buffer.subarray uses (the loop iterating
numOutputs, variables: mode, rpLen, scriptLen, spLen, commitment, rangeProof,
script, assetCommitment, surjectionProof, ephemeralPubkey) so you fail fast on
truncation or unsupported modes and avoid misaligning subsequent entries.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/headers/shielded_outputs.ts (2)

93-106: ⚠️ Potential issue | 🟠 Major

Fail on unknown shielded output modes.

If mode is neither AMOUNT_SHIELDED nor FULLY_SHIELDED, this parser still consumes an ephemeral pubkey and constructs a ShieldedOutput. That desynchronizes the remaining buffer for the next entry instead of rejecting the header immediately.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/headers/shielded_outputs.ts` around lines 93 - 106, The parser currently
only handles ShieldedOutputMode.AMOUNT_SHIELDED and FULLY_SHIELDED and silently
proceeds for other values, causing it to consume ephemeralPubkey and
desynchronize the buffer; update the logic around the mode switch (the code
handling mode, tokenData, assetCommitment, surjectionProof, and ephemeralPubkey)
to explicitly reject unknown modes by throwing an error (or returning a failure)
as soon as mode is not one of ShieldedOutputMode.AMOUNT_SHIELDED or
ShieldedOutputMode.FULLY_SHIELDED, ensuring no buffer consumption occurs and
preventing construction of a ShieldedOutput for invalid modes.

---

53-125: ⚠️ Potential issue | 🟠 Major

Reject truncated shielded headers during parsing.

All fixed-size and length-prefixed reads here slice buf without checking that enough bytes remain first. A truncated payload can therefore produce short commitments/proofs/pubkeys instead of failing fast, which makes malformed wire data look valid.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/headers/shielded_outputs.ts` around lines 53 - 125, The deserialize
method in static deserialize (shielded_outputs.ts) reads fixed-size fields and
length-prefixed blobs without verifying remaining buffer length, so truncated
inputs can produce short slices instead of errors; update static deserialize to
validate buf.length before every fixed-size read (33-byte
commitments/ephemeralPubkey, 1/2-byte header fields) and before each
variable-length read returned by unpackToInt (rpLen, scriptLen, spLen) and throw
a clear Error if insufficient bytes remain; ensure you also validate numOutputs
and any unpacked lengths are non-negative and within remaining buffer bounds
before slicing so ShieldedOutput construction only receives complete buffers.

src/utils/transaction.ts (1)

668-697: ⚠️ Potential issue | 🟠 Major

Validate FULLY_SHIELDED-specific fields before building the model.

This block only checks the shared crypto fields. A FULLY_SHIELDED entry with missing assetCommitment or surjectionProof still becomes a ShieldedOutput and only blows up later when it is serialized or signed.

🛠️ Suggested fix

         const shieldedModels = txData.shieldedOutputs.map(so => {
           if (!so.commitment || !so.rangeProof || !so.script || !so.ephemeralPubkey) {
             throw new Error(
               'Shielded output missing required crypto fields (commitment, rangeProof, script, ephemeralPubkey)'
             );
           }
+          if (
+            so.mode === ShieldedOutputMode.FULLY_SHIELDED &&
+            (!so.assetCommitment || !so.surjectionProof)
+          ) {
+            throw new Error(
+              'Fully shielded output missing required crypto fields (assetCommitment, surjectionProof)'
+            );
+          }
           const tokenData = this.getTokenDataFromOutput(
             {
               type: 'p2pkh',

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/transaction.ts` around lines 668 - 697, The shielded output
constructor currently only checks shared crypto fields; add a mode-specific
validation so that for any txData.shieldedOutputs entry where so.mode ===
'FULLY_SHIELDED' you also verify presence (and non-empty values) of
so.assetCommitment and so.surjectionProof before creating a new ShieldedOutput;
if either is missing throw a clear Error referencing FULLY_SHIELDED,
assetCommitment, and surjectionProof to fail fast (update the validation block
around txData.shieldedOutputs and ShieldedOutput construction).

src/models/shielded_output.ts (1)

101-106: ⚠️ Potential issue | 🟠 Major

Do not serialize an invalid ephemeral pubkey as zeros.

Zero-padding here turns a missing or wrong-sized ephemeral key into a different payload/sighash instead of surfacing the bug. Please fail fast when ephemeralPubkey.length !== EPHEMERAL_PUBKEY_SIZE.

Also applies to: 131-136

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/models/shielded_output.ts` around lines 101 - 106, The serializer
currently silently zero-pads an invalid ephemeralPubkey which hides bugs; update
the serialization logic around ephemeralPubkey and EPHEMERAL_PUBKEY_SIZE so that
if this.ephemeralPubkey is null/undefined you still push
Buffer.alloc(EPHEMERAL_PUBKEY_SIZE) for backward-compat, but if
this.ephemeralPubkey is present and this.ephemeralPubkey.length !==
EPHEMERAL_PUBKEY_SIZE throw a descriptive Error (or assert) instead of
zero-padding; make the same change in the second occurrence handling the
ephemeralPubkey (lines 131-136 equivalent) so both code paths fail fast on
wrong-sized keys.

src/new/wallet.ts (2)

1526-1551: ⚠️ Potential issue | 🔴 Critical

Do not compact newTx.outputs; it breaks output indexes.

Replacing newTx.outputs with only transparent entries renumbers every output after the first shielded one. The rest of the wallet still uses array position as the on-chain output index, so spends and UTXO bookkeeping will point at the wrong output on shielded transactions.

🛠️ Suggested fix

     if (!newTx.shielded_outputs) {
       const shieldedEntries: IHistoryShieldedOutput[] = [];
-      const transparentOutputs: IHistoryOutput[] = [];
       for (const output of newTx.outputs) {
         if (transactionUtils.isShieldedOutputEntry(output)) {
           shieldedEntries.push({
             mode: output.asset_commitment ? 2 : 1, // FullShielded=2, AmountShielded=1
             commitment: output.commitment!, // already hex
@@
             surjection_proof: output.surjection_proof
               ? Buffer.from(output.surjection_proof, 'base64').toString('hex')
               : undefined,
           });
-        } else {
-          transparentOutputs.push(output);
         }
       }
       if (shieldedEntries.length > 0) {
         newTx.shielded_outputs = shieldedEntries;
-        newTx.outputs = transparentOutputs;
       }
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/new/wallet.ts` around lines 1526 - 1551, The current code compacts
newTx.outputs removing shielded entries which shifts on-chain output indexes;
instead, build shielded_entries by iterating newTx.outputs via
transactionUtils.isShieldedOutputEntry (as is done) and assign
newTx.shielded_outputs = shieldedEntries but do NOT reassign or filter
newTx.outputs (leave the original outputs array intact so array indices remain
stable); if you need to mark shielded items for later logic, add a
non-index-affecting marker (e.g., a boolean property) to the output objects
rather than removing them.

---

875-899: ⚠️ Potential issue | 🟠 Major

Return a chain-correct addressPath for shielded addresses.

When opts?.legacy === false, these methods can return a shielded address, but addressPath still comes from getAddressPathForIndex(), which always builds the legacy P2PKH/P2SH path. Any caller that uses the returned path to display or derive the address will get the wrong chain.

src/shielded/processing.ts (1)

77-104: Consider adding debug logging when outputs are silently skipped.

Outputs are silently skipped when:

• address is missing (line 91)
• addressInfo is null (line 95)
• privkey derivation fails (line 104)

While the last case logs at warn level, the first two cases might benefit from debug-level logging to aid troubleshooting when shielded outputs unexpectedly don't appear in wallet balances.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/shielded/processing.ts` around lines 77 - 104, In processShieldedOutputs,
add debug-level logging where outputs are currently silently skipped: when
address is falsy after shieldedOutput.decoded?.address (log shielded output
index idx and any decoded info), and when storage.getAddressInfo(address)
returns null (log address and idx); keep the existing warn for failed privkey
derivation (privkey) but ensure its message includes address, idx and tx
identifier for context. Use the storage.logger (or existing logger) and the same
surrounding variables (shieldedOutput, idx, address, addressInfo, privkey,
transparentCount) so skipped cases are traceable without changing control flow.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/shielded/processing.ts (1)

117-125: ⚠️ Potential issue | 🟠 Major

Validate asset_commitment before rewinding FullShielded outputs.

IHistoryShieldedOutput.asset_commitment is optional, so this still treats malformed fullnode data as a generic rewind failure and silently drops the output. Please warn and continue explicitly before calling rewindFullShieldedOutput().

Suggested fix

       if (isFullShielded) {
         // FullShielded: rewind recovers token UID and asset blinding factor
-        const assetCommitment = Buffer.from(shieldedOutput.asset_commitment!, 'hex');
+        if (!shieldedOutput.asset_commitment) {
+          storage.logger.warn(
+            'FullShielded output missing asset_commitment',
+            tx.tx_id,
+            transparentCount + idx
+          );
+          continue;
+        }
+        const assetCommitment = Buffer.from(shieldedOutput.asset_commitment, 'hex');
         const result = await cryptoProvider.rewindFullShieldedOutput(
           privkey,
           ephPk,

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/shielded/processing.ts` around lines 117 - 125, The code calls
cryptoProvider.rewindFullShieldedOutput for FullShielded outputs without
validating shieldedOutput.asset_commitment; check that
shieldedOutput.asset_commitment exists and is a valid hex string before doing
Buffer.from(...) and before calling rewindFullShieldedOutput(privkey, ephPk,
commitment, rangeProof, assetCommitment); if it's missing or malformed, emit a
warning (e.g., using your logger) that includes identifying context and continue
to the next output instead of attempting the rewind.

src/storage/storage.ts (1)

186-192: ⚠️ Potential issue | 🟠 Major

Allow the cached PIN to be cleared.

This setter only ever writes a plaintext PIN, so a reused Storage instance can retain a usable PIN after shutdown. Please make the argument optional and clear it from handleStop().

Suggested direction

-  setPinCode(pinCode: string): void {
+  setPinCode(pinCode?: string): void {
     this.pinCode = pinCode;
   }

Then call this.setPinCode() during teardown.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/storage/storage.ts` around lines 186 - 192, Change setPinCode(pinCode:
string) to accept an optional value (e.g., setPinCode(pinCode?: string)) and
have it clear the stored PIN when called with undefined (set this.pinCode =
pinCode). Update the Storage instance state so the pinCode field can be
undefined/nullable. In handleStop(), call this.setPinCode() with no argument to
clear the cached PIN during teardown. Ensure references that read this.pinCode
handle the nullable/undefined case appropriately.

src/utils/storage.ts (1)

395-420: ⚠️ Potential issue | 🟠 Major

Gap-limit expansion still regresses to the prefix when shielded is disabled.

shieldedNeedMore is gated correctly, but shieldedTarget and minLastLoaded still consume the inactive shielded cursor. On wallets without shielded keys this makes suffix loading restart from the prefix instead of only fetching new addresses.

Suggested fix

   const legacyTarget = legacyNeedMore ? lastUsedAddressIndex + gapLimit : lastLoadedAddressIndex;
-  const shieldedTarget = shieldedNeedMore
-    ? shieldedLastUsedAddressIndex + gapLimit
-    : shieldedLastLoadedAddressIndex;
+  const shieldedTarget = hasShieldedKeys
+    ? (shieldedNeedMore
+        ? shieldedLastUsedAddressIndex + gapLimit
+        : shieldedLastLoadedAddressIndex)
+    : legacyTarget;
   const maxTarget = Math.max(legacyTarget, shieldedTarget);
-  const minLastLoaded = Math.min(lastLoadedAddressIndex, shieldedLastLoadedAddressIndex);
+  const minLastLoaded = hasShieldedKeys
+    ? Math.min(lastLoadedAddressIndex, shieldedLastLoadedAddressIndex)
+    : lastLoadedAddressIndex;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/storage.ts` around lines 395 - 420, The code uses
shieldedLastLoadedAddressIndex even when shielded keys are absent, causing
minLastLoaded and shieldedTarget to pull the inactive shielded cursor and reset
suffix loading; update the logic to ignore shielded values when hasShieldedKeys
is false: compute shieldedTarget using hasShieldedKeys (if false, set
shieldedTarget to lastLoadedAddressIndex) and compute minLastLoaded as
hasShieldedKeys ? Math.min(lastLoadedAddressIndex,
shieldedLastLoadedAddressIndex) : lastLoadedAddressIndex, so
shieldedLastLoadedAddressIndex is only considered when hasShieldedKeys is true
(affecting shieldedNeedMore, shieldedTarget, and minLastLoaded).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/utils/transaction.ts (1)

667-695: ⚠️ Potential issue | 🟠 Major

Validate FULLY_SHIELDED-only fields before instantiating the model.

The current guard only checks the shared fields. A FULLY_SHIELDED output can still be built with assetCommitment or surjectionProof missing and then fail later during serialization or processing instead of failing fast here.

🧪 Fail fast on mode-specific fields

         const shieldedModels = txData.shieldedOutputs.map(so => {
           if (!so.commitment || !so.rangeProof || !so.script || !so.ephemeralPubkey) {
             throw new Error(
               'Shielded output missing required crypto fields (commitment, rangeProof, script, ephemeralPubkey)'
             );
           }
+          if (
+            so.mode === 'FULLY_SHIELDED' &&
+            (!so.assetCommitment || !so.surjectionProof)
+          ) {
+            throw new Error(
+              'Fully shielded output missing required crypto fields (assetCommitment, surjectionProof)'
+            );
+          }
           const tokenData = this.getTokenDataFromOutput(

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/transaction.ts` around lines 667 - 695, The guard in the
shieldedModels mapping only checks shared fields but doesn't enforce
mode-specific requirements: before instantiating ShieldedOutput in the
txData.shieldedOutputs -> shieldedModels block, add a check for outputs with
so.mode === 'FULLY_SHIELDED' to ensure so.assetCommitment and so.surjectionProof
are present (non-null/defined) and throw a clear Error if missing; keep existing
checks for commitment, rangeProof, script, ephemeralPubkey and perform the
FULLY_SHIELDED checks prior to calling this.getTokenDataFromOutput and new
ShieldedOutput so failures occur fast and deterministically.

src/utils/wallet.ts (2)

710-729: ⚠️ Potential issue | 🔴 Critical

Apply the same multisig guard to the seed-based path.

generateAccessDataFromSeed() has the same issue: multisig wallets still derive and store single-sig shielded keys from the root seed. That creates a second, incompatible signing authority alongside the multisig account.

🔒 Safer guard

-    const scanAcctXpriv = rootXpriv.deriveNonCompliantChild(SHIELDED_SCAN_ACCT_PATH);
-    const scanXpriv = scanAcctXpriv.deriveNonCompliantChild(0);
-    const spendAcctXpriv = rootXpriv.deriveNonCompliantChild(SHIELDED_SPEND_ACCT_PATH);
-    const spendXpriv = spendAcctXpriv.deriveNonCompliantChild(0);
+    let scanXpriv: HDPrivateKey | undefined;
+    let spendXpriv: HDPrivateKey | undefined;
+    if (walletType === WalletType.P2PKH) {
+      const scanAcctXpriv = rootXpriv.deriveNonCompliantChild(SHIELDED_SCAN_ACCT_PATH);
+      scanXpriv = scanAcctXpriv.deriveNonCompliantChild(0);
+      const spendAcctXpriv = rootXpriv.deriveNonCompliantChild(SHIELDED_SPEND_ACCT_PATH);
+      spendXpriv = spendAcctXpriv.deriveNonCompliantChild(0);
+    }
@@
-      scanXpubkey: scanXpriv.xpubkey,
-      scanMainKey: encryptData(scanXpriv.xprivkey, pin),
-      spendXpubkey: spendXpriv.xpubkey,
-      spendMainKey: encryptData(spendXpriv.xprivkey, pin),
+      ...(scanXpriv && {
+        scanXpubkey: scanXpriv.xpubkey,
+        scanMainKey: encryptData(scanXpriv.xprivkey, pin),
+      }),
+      ...(spendXpriv && {
+        spendXpubkey: spendXpriv.xpubkey,
+        spendMainKey: encryptData(spendXpriv.xprivkey, pin),
+      }),

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/wallet.ts` around lines 710 - 729, In generateAccessDataFromSeed(),
avoid deriving and storing single-sig shielded keys when multisigData is
present: add a guard checking multisigData before using scanAcctXpriv/scanXpriv
and spendAcctXpriv/spendXpriv (the derivation and encryptData calls), and if
multisigData exists skip those derivations and set
scanXpubkey/scanMainKey/spendXpubkey/spendMainKey to null/undefined (or the
multisig-appropriate placeholders) so you do not create a second single-sig
signing authority; mirror the multisig check used in the other wallet creation
path to keep behavior consistent.

---

627-636: ⚠️ Potential issue | 🔴 Critical

Skip shielded key derivation for multisig wallets here.

This block still runs for WalletType.MULTISIG, so a multisig root key ends up persisting standalone shielded scan/spend chains derived from a single signer. That breaks the wallet’s signing model by making a multisig wallet look shielded-capable with one-party keys.

🔒 Safer guard

-    if (argXpriv.depth === 0) {
+    if (argXpriv.depth === 0 && walletType === WalletType.P2PKH) {
       const scanAcctXpriv = argXpriv.deriveNonCompliantChild(SHIELDED_SCAN_ACCT_PATH);
       const scanXpriv = scanAcctXpriv.deriveNonCompliantChild(0);
       accessData.scanXpubkey = scanXpriv.xpubkey;
       accessData.scanMainKey = encryptData(scanXpriv.xprivkey, pin);

       const spendAcctXpriv = argXpriv.deriveNonCompliantChild(SHIELDED_SPEND_ACCT_PATH);
       const spendXpriv2 = spendAcctXpriv.deriveNonCompliantChild(0);
       accessData.spendXpubkey = spendXpriv2.xpubkey;
       accessData.spendMainKey = encryptData(spendXpriv2.xprivkey, pin);
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/wallet.ts` around lines 627 - 636, The shielded key derivation
block currently runs even for multisig roots and must be skipped for
WalletType.MULTISIG; wrap the existing code that derives
SHIELDED_SCAN_ACCT_PATH/SHIELDED_SPEND_ACCT_PATH and sets
accessData.scanXpubkey/scanMainKey/spendXpubkey/spendMainKey with a guard that
checks the wallet type (e.g., if (argXpriv.depth === 0 && walletType !==
WalletType.MULTISIG) or use the local isMultisig flag), so multisig root keys do
not generate or persist standalone shielded scan/spend chains; leave encryptData
usage intact for non-multisig flows.

src/sync/stream.ts (1)

542-605: ⚠️ Potential issue | 🟠 Major

isProcessingQueue can still get stuck on non-derivation failures.

The new try/catch only protects the shielded-derivation block. If saveAddress(addr), isAddressMine(), addTx(), or another awaited call throws, processQueue() still exits before clearing the flag, and shutdown() falls back to the timeout path.

Wrap the whole loop in try/finally and set abortWithError(...) from the outer catch path so queue teardown is reliable even when item processing fails.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/sync/stream.ts` around lines 542 - 605, The processQueue loop can leave
isProcessingQueue true if any awaited call (e.g., storage.isAddressMine,
storage.saveAddress, storage.addTx, deriveShieldedAddressFromStorage) throws;
wrap the entire while(true) processing loop in a try/finally so
isProcessingQueue is always cleared in the finally block, and add an outer catch
that calls abortWithError(error) before rethrowing or breaking to ensure
shutdown() does not fall back to the timeout path; update processQueue (the
method) to catch errors from any item processing, call abortWithError(err) (or
set the same failure path used by shutdown) in that catch, and then allow the
finally to reset this.isProcessingQueue.

src/types.ts (1)

761-765: ⚠️ Potential issue | 🟠 Major

Reflect optional shielded private keys in the interface.

scanMainKey and spendMainKey are optional in IWalletAccessData, so these accessors cannot always return string for pre-shielded wallets. The current signature hides that state from callers and pushes them into runtime failure paths instead of typed handling.

Suggested fix

-  getScanXPrivKey(pinCode: string): Promise<string>;
-  getSpendXPrivKey(pinCode: string): Promise<string>;
+  getScanXPrivKey(pinCode: string): Promise<string | undefined>;
+  getSpendXPrivKey(pinCode: string): Promise<string | undefined>;
   getScanXPubKey(): Promise<string | undefined>;
   getSpendXPubKey(): Promise<string | undefined>;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/types.ts` around lines 761 - 765, The getters getScanXPrivKey and
getSpendXPrivKey currently promise a string but IWalletAccessData marks
scanMainKey and spendMainKey as optional, so update the signatures of
getScanXPrivKey(pinCode: string) and getSpendXPrivKey(pinCode: string) to return
Promise<string | undefined>, and update any implementations (the methods that
read scanMainKey / spendMainKey) to return undefined when those keys are absent
and adjust callers to handle the undefined case (e.g., early-return, throw with
a clear message, or conditional logic). Ensure the public
getScanXPubKey/getSpendXPubKey remain Promise<string | undefined> as already
declared.

src/utils/storage.ts (2)

855-867: ⚠️ Potential issue | 🟠 Major

Use shielded_spend consistently here.

These branches still look for shielded-spend, so shielded spend addresses fall through the transparent path and shieldedMaxIndexUsed never advances.

Based on learnings, deriveShieldedAddressFromStorage() persists shielded spend addresses as shielded_spend.

Also applies to: 967-979

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/storage.ts` around lines 855 - 867, The branch is checking for the
wrong addressType token ('shielded-spend') so shielded spend addresses are
misclassified; update all checks that compare addressInfo.addressType for
shielded spend (including the comparison near the shieldedMaxIndexUsed update
and the similar logic later) to use 'shielded_spend' (the same token used by
deriveShieldedAddressFromStorage()) so shieldedMaxIndexUsed is correctly
advanced instead of falling through to the legacy path; make sure any other
comparisons in this file that look for 'shielded-spend' are changed to
'shielded_spend' and run tests/linters.

---

787-828: ⚠️ Potential issue | 🔴 Critical

Make shielded-output merging idempotent and index-stable.

processHistory() can revisit the same tx, but this block appends decoded shielded entries every time and then saves the enlarged transaction back to storage. On the next pass, tx.outputs.length is already inflated, so result.index - transparentCount no longer maps to the original tx.shielded_outputs slot and you start creating duplicate balances/UTXOs off synthetic indexes.

🛠️ Suggested fix

-      // Capture transparent output count before appending decoded shielded outputs.
-      // result.index from processShieldedOutputs uses this same count as base,
-      // so (result.index - transparentCount) gives the shielded_outputs array index.
-      const transparentCount = tx.outputs.length;
+      const transparentOutputs = tx.outputs.filter(
+        output => !transactionUtils.isShieldedOutputEntry(output)
+      );
 
       const shieldedResults = await processShieldedOutputs(
         storage,
         tx,
         storage.shieldedCryptoProvider,
         pinCode
       );
-      for (const result of shieldedResults) {
+      tx.outputs = transparentOutputs;
+      for (const [idx, result] of shieldedResults.entries()) {
         const walletTokenUid =
           result.tokenUid === NATIVE_TOKEN_UID_HEX ? NATIVE_TOKEN_UID : result.tokenUid;
-        const so = tx.shielded_outputs[result.index - transparentCount];
+        const so = tx.shielded_outputs[idx];

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/utils/storage.ts` around lines 787 - 828, The current merge appends
decoded shielded outputs every pass which inflates tx.outputs and breaks index
mapping; to fix, compute a stable base count of transparent outputs (e.g., const
baseTransparentCount = tx.outputs.filter(o => o.type !== 'shielded').length)
before calling processShieldedOutputs, then for each result compute soIndex =
result.index - baseTransparentCount and only append the decoded shielded output
if it is not already present (compare a unique field like commitment or
decoded/address) — and only call store.saveTx(tx) if you actually appended any
new entries; update the logic around processShieldedOutputs, tx.outputs pushing,
and store.saveTx accordingly to make merging idempotent and index-stable.

src/new/wallet.ts (2)

1543-1568: ⚠️ Potential issue | 🔴 Critical

Keep newTx.outputs index-stable when extracting shielded entries.

Replacing newTx.outputs with transparentOutputs renumbers every transparent output after the first shielded one. The rest of the wallet still uses array position as the on-chain output index, so spends and UTXO bookkeeping for shielded transactions will point at the wrong output.

🛠️ Suggested fix

     if (!newTx.shielded_outputs) {
       const shieldedEntries: IHistoryShieldedOutput[] = [];
-      const transparentOutputs: IHistoryOutput[] = [];
       for (const output of newTx.outputs) {
         if (transactionUtils.isShieldedOutputEntry(output)) {
           shieldedEntries.push({
             mode: output.asset_commitment ? 2 : 1, // FullShielded=2, AmountShielded=1
             commitment: output.commitment, // already hex
@@
             surjection_proof: output.surjection_proof
               ? Buffer.from(output.surjection_proof, 'base64').toString('hex')
               : undefined,
           });
-        } else {
-          transparentOutputs.push(output);
         }
       }
       if (shieldedEntries.length > 0) {
         newTx.shielded_outputs = shieldedEntries;
-        newTx.outputs = transparentOutputs;
       }
     }

---

861-865: ⚠️ Potential issue | 🟠 Major

Resolve shielded paths from the stored address type, not only from opts.

This only returns a shielded path when the caller already knows to pass { legacy: false }. Internal APIs that derive addressPath from stored addresses/UTXOs (getAvailableUtxos(), getWalletInputInfo(), getSignatures()) still call getAddressPathForIndex(index) without that hint, so shielded-spend addresses keep surfacing with transparent derivation paths.

src/new/types.ts (1)

330-344: Encode the shielded-address invariant in the type.

The JSDoc now says address must be shielded when shielded is set, but ProposedOutput still accepts any string in both cases. That means invalid output combinations still compile and only fail later. A discriminated union for transparent vs shielded outputs would make this API much harder to misuse.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/new/types.ts` around lines 330 - 344, Change ProposedOutput into a
discriminated union so the compiler enforces the invariant that shielded outputs
require a shielded address: define two interfaces (e.g.
TransparentProposedOutput and ShieldedProposedOutput) that share common fields
(value: OutputValueType, token: string, timelock?: number) but differ on the
discriminator: TransparentProposedOutput has shielded?: undefined (or absent)
and address: string (transparent address), while ShieldedProposedOutput has
shielded: ShieldedOutputMode and address: string (or a new ShieldedAddress type
if available) and then export type ProposedOutput = TransparentProposedOutput |
ShieldedProposedOutput; update references to ProposedOutput accordingly.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes